Fix various issues - #11029
Conversation
|
There was a problem hiding this comment.
🟡 Not ready to approve
It includes an accidentally committed .rej artifact and CMake still needs to export/check HAVE_SYS_UN_H to avoid public ABI/layout mismatches for consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR addresses multiple correctness and robustness issues across certificate name/IP checking, PKCS#7 SignedData streaming verification, and big-integer cleanup paths, with accompanying API tests and build-system adjustments to keep public header feature macros consistent between library and consumers.
Changes:
- Hardened PKCS#7 SignedData streaming parsing to avoid accepting truncated footers and to correctly error when required
signerInfosis missing; added tests to cover the cases. - Updated IP alt-name checking to respect the caller-provided length (avoid reliance on NUL termination) and added an OpenSSL-compat test for bounded inputs.
- Made several bigint cleanup paths safer by zero-initializing heap-allocated
mp_intstructs when initialization may be skipped; updated build configuration to exportHAVE_SYS_TIME_H.
File summaries
| File | Description |
|---|---|
| wolfssl/internal.h | Extends internal CheckIPAddr prototype to accept an explicit input length. |
| src/internal.c | Updates CheckIPAddr implementation and internal call sites to pass explicit lengths. |
| src/x509.c | Updates X509 host/IP checking to pass bounded lengths into IP-alt-name checks. |
| tests/api/test_ossl_x509.c | Adds a bounded (non-NUL-terminated) host-name test to verify length-respecting behavior. |
| wolfcrypt/src/pkcs7.c | Adjusts streaming stage windowing/capping to avoid stalling and to reject missing signerInfos. |
| tests/api/test_pkcs7.c | Removes NO_PKCS7_STREAM gating for a truncation test; adds a new “no signerInfos” regression test. |
| tests/api/test_pkcs7.h | Declares/registers the new PKCS#7 regression test. |
| wolfcrypt/src/srp.c | Zero-initializes heap-allocated mp_int temporaries so cleanup is safe when init is skipped. |
| wolfcrypt/src/ecc.c | Zero-initializes mp_int temporaries in a verification-faults path so cleanup is safe on early error. |
| wolfcrypt/src/rsa.c | Zero-initializes heap-allocated mp_int temporaries so cleanup is safe when init is skipped. |
| wolfcrypt/src/rsa.c.rej | Adds a patch reject artifact file in-tree (should not be committed). |
| configure.ac | Adjusts header checks and exports HAVE_SYS_UN_H/HAVE_SYS_TIME_H via CPP flags for public header layout consistency. |
| CMakeLists.txt | Ensures in-tree CMake builds propagate HAVE_SYS_TIME_H to match installed headers. |
| cmake/options.h.in | Exports HAVE_SYS_TIME_H via generated options header for consumers. |
| cmake/config.in | Removes HAVE_SYS_TIME_H from CMake-generated config header (now exported via options header). |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
7d62180 to
e03197e
Compare
|
Retest this please |
e03197e to
5c3f92e
Compare
Thanks to Christos Papakonstantinou (Cantina Security) for the report.
Thanks to Christos Papakonstantinou (Cantina Security) for the report.
…rary. Thanks to Christos Papakonstantinou (Cantina Security) for the report.
Thanks to Christos Papakonstantinou (Cantina Security) for the report.
803d3c1 to
4aba977
Compare
|
Jenkins retest this please |
|
|
||
| /* check if bundle has more elements or footer, if not, set content | ||
| * to pkcs7->content and hash to pkcs7->hash. | ||
| /* expect data length to be enough to check set and seq of certs, |
There was a problem hiding this comment.
Recommendation: Do not merge as-is. The residual-byte expression (maxLen - totalRd) + stream->length is not a valid measure of "bytes left to parse" at the point stage 3 hands off: stage 3 does not call wc_PKCS7_StreamEndCase() before computing it, so totalRd (advanced when bytes are copied into the stream buffer) and stream->length (bytes still buffered, some already consumed by wc_PKCS7_HandleOctetStrings) are not in sync, and the resulting expected can be smaller or larger than what stages 4/5 actually need. Either sync the stream bookkeeping before computing the residual, or keep the residual purely as an emptiness test (remaining == 0 -> PKCS7_NO_SIGNER_E) and reinstate master's behavior for a short-but-nonzero footer instead of clamping the stage-4 window to it. Whichever route, add the chunk-size sweep above -- the current one-shot-only tests cannot see this class of bug.
| /* chk is length delimited and may not be NUL terminated, so check it | ||
| * against the iPAddress entries directly rather than through the | ||
| * NUL terminated CheckIPAddr helper. */ | ||
| ret = CheckHostName(dCert, (char *)chk, chklen, 0, 1); |
There was a problem hiding this comment.
Recommendation: Route this through CheckIPAddr() now that it accepts an explicit length, and delete the stale comment. If the direct CheckHostName() call is deliberate for some other reason, update the comment to state the real reason.
Description
Fixes zd#22229
Testing
Built in tests, provided reproducers
Checklist